create safer isValidAddress method #11089
Conversation
Builds ready [88fdb55]
Page Load Metrics (569 ± 51 ms)
|
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
88fdb55
to
bf90736
Compare
Builds ready [bf90736]
Page Load Metrics (575 ± 33 ms)
|
jsconfig.json
Outdated
| "node_modules", | ||
| "patches", | ||
| "test-artifacts" | ||
| ] |
There was a problem hiding this comment.
were these exclusions no longer needed here or?
There was a problem hiding this comment.
oh, not part of this i'll remove. With them in autocomplete just randomly starts failing for me.
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
bf90736
to
b4c496d
Compare
Builds ready [b4c496d]
Page Load Metrics (602 ± 33 ms)
|
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
b4c496d
to
8dc1659
Compare
|
I discovered that our internal 'isHex' tool also doesn't validate that the hex string starts with '0x' so the previous fix by @tmashuang (#11071) was not sufficient to prevent errors for real addresses that aren't hex prefixed, so this PR replaces the usages of that with isValidHexAddress (in no case where it was used did we expect anything other than an address. Note that Thomaas' change does prevent issues when non hex strings (like domain names) were sent to the icons. Also reverted the change that made non 0x prefixed hex strings invalid for the purposes of this method but we should explore changing this behavior in the future. |
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
8dc1659
to
3519fb3
Compare
|
One more tweak, i went through all usages of isValidAddress (both our util and the ethereumjs-util function) and restored it's rightful behavior by adding a new prop to the new helper called 'allowNonPrefixed' ...why? ethereumjs-util.isValidAddress prior to my update would return false for non hex prefixed strings, whereas our method would return true provided that it was a 40 character hex string. This new prop allows us to have both behaviors in one method, and a path for gradual migration of using non hex prefixed values and allowing them from users. |
Builds ready [3519fb3]
Page Load Metrics (640 ± 62 ms)
|
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
414c048
to
f0140d6
Compare
There was a problem hiding this comment.
I like the new function, and the consolidation of all of those utility functions. But it seems that there are a lot of potentially dapp-breaking changes in here still, and a few undesirable changes to the UI.
It makes perfect sense to me that we'd ensure our dapp interactions use properly-formatted addresses, but when making any non-backwards-compatible changes to our provider (however reasonable) we should consider how best to communicate these changes before making them. Giving plenty of advanced notice, and maybe shipping a deprecation notice ahead of time, would be much friendlier to devs. Especially in cases like this where there are few downsides for us to preserving this behaviour a little longer.
And regarding checksumming, we should consider adopting support for EIP-1191, and considering addresses checksummed by EIP-1191 or by EIP-55 as valid. Or alternatively, we could skip validating mixed-case addresses as checksummed for now (or not extending our validation further at least). I'd hate to see us accidentally stand in the way of a new, helpful EIP becoming more widespread.
ui/helpers/utils/icon-factory.js
Outdated
| @@ -18,7 +19,7 @@ function IconFactory(jazzicon) { | |||
| IconFactory.prototype.iconForAddress = function (address, diameter) { | |||
| let addr = address; | |||
|
|
|||
| if (isHex(address)) { | |||
| if (isValidHexAddress(address, false)) { | |||
There was a problem hiding this comment.
This seems dangerous. Previously checksumAddress would add the prefix, and checksum the address regardless of whether it's mixed-case or not. Now it requires the address is already hex-prefixed and either checksummed already or all one case. This means for any other case, we're skipping this normalization step, and we'll be generating a different icon than before.
There was a problem hiding this comment.
under the hood, the utility method 'checksumAddress' calls ethereumjs-util's toChecksumAddress which in 5.1 adds the '0x' prefix, and in the new version, it throws an error. It seems like we should probably add a new utility method that mimics this behavior because as is even when allowing this to validate true for a non hex prefixed string, the string is not modified meaning it would immediately throw on the next line.
I'm going to add this utility method now, and replace usages of the old helper method.
There was a problem hiding this comment.
actually I should do that in a new PR instead to prevent convoluting this with new logic
| @@ -167,7 +165,7 @@ class AddToken extends Component { | |||
| autoFilled: false, | |||
| }); | |||
|
|
|||
| const addressIsValid = isValidAddress(customAddress); | |||
| const addressIsValid = isValidHexAddress(customAddress, false); | |||
There was a problem hiding this comment.
This behavior is pre-existing, but it does seem strange to me that we'd show an "invalid address" error when the address is mixed-case but doesn't match our checksum. Particularly when we're not checksumming the addresses properly according to EIP-1191, as we're omitting the chainID. As a result, we'd be showing "invalid address" when an address was properly checksummed in accordance with EIP-1191.
| @@ -101,7 +104,7 @@ export default class AddRecipient extends Component { | |||
|
|
|||
| let content; | |||
|
|
|||
| if (isValidAddress(query)) { | |||
| if (!isBurnAddress(query) && isValidHexAddress(query)) { | |||
There was a problem hiding this comment.
It seems overly harsh to disallow differently-checksummed addresses here.
The same goes for the other uses of this function in the Send flow and the contacts flow.
There was a problem hiding this comment.
While that is true, it is the previously implemented behavior. Should I change that as part of this PR? These places used the previous 'isValidAddress' custom function which did check for mixed case and do validation using isValidChecksum.
There was a problem hiding this comment.
It would be perfectly reasonable to stick with targeting pre-v9.5.0 behavior IMO, and leave this behaving as it did before. Particularly since this is destined for a hotfix.
Builds ready [f0140d6]
Page Load Metrics (732 ± 52 ms)
|
|
@Gudahtt my goal was to preserve the behavior pre 9.5.0 and the |
|
Sure, of course! That solution sounds perfect to me. |
|
There are no futher usages of either:
in all cases to restore pre 9.5 behavior the new isValidHexAddress should be called in the following ways: I did an audit of all places where isValidHexAddress is invoked (29 invocations)
There are two files that deserve special mention:
the new usages of this utility method in the above two instances will be removed in a new PR adding the toChecksumHexAddress. |
This comment has been minimized.
This comment has been minimized.
brad-decker
force-pushed
the
use-safer-is-valid-address-
branch
from
df861b5
to
12b3631
Compare
Builds ready [12b3631]
Page Load Metrics (592 ± 44 ms)
|
Builds ready [8dcd521]
Page Load Metrics (619 ± 53 ms)
|
There was a problem hiding this comment.
Everything looks good aside from these two identicon-related changes. I suggested that we revert changes for now so that we're not making things any worse, until we're prepared to fix those properly. But let me know if you think there's a good reason to keep these changes.
Builds ready [d568288]
Page Load Metrics (636 ± 53 ms)
|
|
thank you thank you thank you, @Gudahtt. 🙏🏻 🎉 💯 🥇 🚀 |
Builds ready [63861cb]
Page Load Metrics (611 ± 36 ms)
|
Instead of using the
isValidAddressmethod fromethereumjs-utilthat now throws when a non hex prefixed string is provided, or the overloadedisValidAddresshelper we maintain that checks a number of things, we should use a more effective shared method.This PR creates two new shared methods:
The tests from our 'isValidAddress' implementation have been copied over and only ONE has been altered fundamentally. This new method does not return true for forty-character non hex-prefixed strings. If user input would be valid without the hex prefix, then it should be hex prefixed before calling this method.